Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for styling lines and shapes in Select One from Map questions #6083

Merged
merged 23 commits into from
Apr 26, 2024
Merged

Add support for styling lines and shapes in Select One from Map questions #6083

merged 23 commits into from
Apr 26, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Apr 11, 2024 •
edited
Loading

Closes #5945

Why is this the best possible solution? Were any other approaches considered?

This is a continuation of #5278 and the approach is also the same so there is nothing important to discuss here.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

As described in the issue we are introducing new ways of styling map elements (lines and polygons). Everything that's map-related needs to be tested with all three providers google, mapbox, and osm.
The only problem is that there is no way to set the width of a line in polygons in Mapbox. It's just not supported at this moment. We could have tried to deal with it somehow by drowning additional lines on top of the borders of a polygon but I don't think it's necessary. Let's wait for Mapbox to add this option.

Do we need any specific form for testing your changes? If so, please attach one.

I've used:
s1FromMap.xlsx
items.geojson.txt

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes getodk/docs#1780

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 force-pushed the COLLECT-5945 branch 4 times, most recently from 17f5071 to b2ec0a4 Compare April 11, 2024 17:53
@grzesiek2010 grzesiek2010 marked this pull request as ready for review April 12, 2024 08:40
@grzesiek2010 grzesiek2010 requested a review from seadowg April 12, 2024 08:40
@grzesiek2010 grzesiek2010 mentioned this pull request Apr 12, 2024
Merged
seadowg
seadowg requested changes Apr 15, 2024
View reviewed changes
androidshared/src/main/java/org/odk/collect/androidshared/utils/ColorUtils.kt Outdated Show resolved Hide resolved
...ct_app/src/main/java/org/odk/collect/android/widgets/items/SelectOneFromMapDialogFragment.kt Outdated Show resolved Hide resolved
geo/src/main/java/org/odk/collect/geo/geopoly/GeoPolyActivity.java Show resolved Hide resolved
mapbox/src/main/java/org/odk/collect/mapbox/DynamicPolyLineFeature.kt Outdated Show resolved Hide resolved
mapbox/src/main/java/org/odk/collect/mapbox/StaticPolyLineFeature.kt Outdated Show resolved Hide resolved
maps/src/main/java/org/odk/collect/maps/LineDescription.kt Outdated Show resolved Hide resolved
maps/src/main/java/org/odk/collect/maps/LineDescription.kt Outdated Show resolved Hide resolved
maps/src/test/java/org/odk/collect/maps/LineDescriptionTest.kt Outdated Show resolved Hide resolved
geo/src/main/java/org/odk/collect/geo/selection/SelectionSummarySheet.kt Outdated Show resolved Hide resolved
@grzesiek2010 grzesiek2010 requested a review from seadowg April 15, 2024 20:40
seadowg
seadowg requested changes Apr 16, 2024
View reviewed changes
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one small tweak. I'll mark this ready for testing now.

mapbox/src/main/java/org/odk/collect/mapbox/DynamicPolyLineFeature.kt Outdated Show resolved Hide resolved
@dbemke
Copy link

dbemke commented Apr 18, 2024 •
edited
Loading

It's impossible to add points in Geotrace widget (all input methods, all map sources)

Steps to reproduce:

  1. Go to All widgets form to Geotrace widget.
  2. Tap "Get line”.
  3. Choose any Input method and try to add a point.

The issue doesn’t occur on the master version 213624d and the store version

@grzesiek2010 grzesiek2010 requested a review from seadowg April 18, 2024 21:41
@grzesiek2010
Copy link
Member Author

It's impossible to add points in Geotrace widget (all input methods, all map sources)

The issue is fixed.

@dbemke
Copy link

dbemke commented Apr 19, 2024

In Google maps the color of the background in shapes and the outline seems to fade while zooming in when the shape covers a building on a map. The outline that is beyond a building doesn't fade. Other sources of maps don't work in that way.

googleBuilding.mp4

@dbemke
Copy link

dbemke commented Apr 19, 2024

Should the outline/stroke in a shapes in Mapbox work like the other sources of maps after the fix?

@dbemke
Copy link

dbemke commented Apr 19, 2024

I'm also not sure what should be the expected result if a stroke width is set and I zoom in and out - should it scale? e.g. if the outline is as wide as a street and I zoom out should it keep the width of a street after zooming out?

scale.mp4

@grzesiek2010
Copy link
Member Author

In Google maps the color of the background in shapes and the outline seems to fade while zooming in when the shape covers a building on a map. The outline that is beyond a building doesn't fade. Other sources of maps don't work in that way.

This issue is not caused by this particular pull request, but rather appears to be default behavior in Google Maps. Consequently, I don't think we should alter it. The key distinction here lies not in the fading of colors, but in the fact that Google Maps displays 3D buildings, whereas Mapbox and OSM do not. Should we consider changing it? My stance remains unchanged I don't believe it's necessary. While it's important to ensure compatibility across different map providers, we needn't strive for absolute uniformity at any cost.

@grzesiek2010
Copy link
Member Author

I'm also not sure what should be the expected result if a stroke width is set and I zoom in and out - should it scale? e.g. if the outline is as wide as a street and I zoom out should it keep the width of a street after zooming out?

It should scale. Otherwise, it would disappear after zooming out if for example as you said it was the size of a street.

@grzesiek2010
Copy link
Member Author

Should the outline/stroke in a shapes in Mapbox work like the other sources of maps after the fix?

I have forgotten to mention (the description is now updated):

The only problem is that there is no way to set the stroke width in polygons in Mapbox (we can do that for lines only). It's just not supported at this moment. We could have tried to deal with it somehow by drowning additional lines on top of the borders of a polygon but I don't think it's necessary. Let's wait for Mapbox to add this option.

@seadowg
Copy link
Member

seadowg commented Apr 23, 2024

Given @getodk/testers have already started testing on the PR branch, I'll hold off merging this for now.

@dbemke
Copy link

dbemke commented Apr 26, 2024

@seadowg If you have time you can merge it cause it's easier for us to test everything at the same time (as at the moment there are a few PR waiting to be tested and we're a bit short of time).

@dbemke
Copy link

dbemke commented Apr 26, 2024

Tested with Success!

Verified on devices with Android 10, 14

Verified Cases:

@WKobus
Copy link

WKobus commented Apr 26, 2024

Tested with Success!

Verified on device with Android 11

@seadowg seadowg merged commit ac23a4f into getodk:master Apr 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seadowg seadowg seadowg approved these changes

Assignees
No one assigned
Labels
behavior verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for styling lines and shapes in Select One from Map questions
4 participants
@grzesiek2010 @dbemke @seadowg @WKobus